-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address issues of missing bucket_name
in s3fs
paths
#673
Address issues of missing bucket_name
in s3fs
paths
#673
Conversation
bucket_name
in s3fs pathsbucket_name
in s3fs
paths
bucket_name
in s3fs
pathsbucket_name
in s3fs
paths
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #673 +/- ##
=======================================
Coverage 83.59% 83.60%
=======================================
Files 79 79
Lines 6230 6233 +3
=======================================
+ Hits 5208 5211 +3
Misses 1022 1022 ☔ View full report in Codecov by Sentry. |
.github/workflows/ci.yml
Outdated
@@ -85,6 +86,8 @@ jobs: | |||
S3_SECRET_KEY: ${{ secrets.s3_secret_key }} | |||
S3_ENDPOINT: https://s3.sbg.cloud.ovh.net/ | |||
S3_REGION: sbg | |||
S3_BUCKET_NAME: quetz | |||
QUETZ_S3_BUCKET_NAME: quetz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you brielfy elaborate what's the purpose of this env var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I found out with fsspec/s3fs#824, we need to specify the bucket_name in the paths for s3fs, we need to know how the bucket is called that we write to. Previously (at least for aws s3) this information could be encoded in S3_ENDPOINT
, however, f3fs doesn't work with that (bucket cannot be encoded in URL). Therefore we need another variable explicitly stating the bucket_name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why do we need S3_BUCKET_NAME
and QUETZ_S3_BUCKET_NAME
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already removed QUETZ_S3_BUCKET_NAME
again, I just tested the QUETZ_
prefix to make sure that it wouldn't be mapped to S3_BUCKET_NAME
in the tests (as the env variable was missing)
The tests are failing because it's using the CI definition from the main branch. |
Motivation
The previous way of specifying paths for
s3fs
(not including the bucket_name) led to issues when usingendpoint_url
.Changes
As paths in the format of
<root_dir>/<subdir>/.../<file>
are not officially supported bys3fs
(see fsspec/s3fs#824), we are now using<bucket_name>/<root_dir>/<subdir>/.../<file>
.For that, we need a
bucket_name
config field.def _bucket_map(...)
is adjusted according so that every channel specification also includes thebucket_name